Support canonical ledger entry names #5271

bthomee · 2025-02-01T21:02:50Z

High Level Overview of Change

This change implements #4393, which enhances the filtering in the ledger, ledger_data, and account_objects methods by also supporting filtering by the canonical name of the LedgerEntryType using case-insensitive matching.

Context of Change

Currently the mapping between the RPC name to provide as filter and the LedgerEntryType is inconsistent and sometimes surprising. For example, to match the DirectoryNode the provided RPC name is directory, while for PayChannel it is payment_channel.

This change enables filtering on the canonical name as well to make the mapping consistent and unsurprising. For instance, to filter on PayChannel it is still possible to specify payment_channel, but now it's also possible to specify PayChannel and paychannel (or any variant thereof using different casing).

Type of Change

New feature (non-breaking change which adds functionality)

API Impact

Public API: New feature (new methods and/or new fields)

Before / After

See #4393 for an excellent description.

codecov · 2025-02-01T21:24:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.2%. Comparing base (dc9e6c3) to head (7a35881).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5271   +/-   ##
=======================================
  Coverage     78.1%   78.2%           
=======================================
  Files          790     790           
  Lines        67652   67652           
  Branches      8165    8165           
=======================================
+ Hits         52870   52874    +4     
+ Misses       14782   14778    -4

Files with missing lines	Coverage Δ
src/xrpld/rpc/detail/RPCHelpers.cpp	`82.8% <100.0%> (ø)`

... and 3 files with indirect coverage changes

mvadari · 2025-02-03T17:17:38Z

Why deprecate the old filter names? Why not support both?

bthomee · 2025-02-03T19:03:04Z

Why deprecate the old filter names? Why not support both?

@mDuo13 can you respond? In the issue you had raised you mentioned "For backwards compatibility, continue to also support the existing short names (but mark them deprecated in the documentation)." - are there requests from the community to deprecate that field to reduce confusion, or is there another reason?

mDuo13 · 2025-02-03T20:49:02Z

The reason to mark it deprecated is so that we can move away from needing a short name for new ledger entries (a dev task that was often forgotten in the past anyway). There should be one preferred way to look up ledger entry types, by their canonical name (case-insensitive).

It's fine if the existing, "deprecated" shortcut names are supported indefinitely too.

mvadari · 2025-02-05T19:04:28Z

Maybe this is an unpopular opinion but I actually like the lower-case names, and think we should keep them.

note: I have no objection to adding additional filter names, just to deprecating/replacing the existing setup.

bthomee · 2025-02-05T19:34:58Z

Maybe this is an unpopular opinion but I actually like the lower-case names, and think we should keep them.

note: I have no objection to adding additional filter names, just to deprecating/replacing the existing setup.

I have no particular opinion here or sufficient background to know how people use this API, but do note that this PR explicitly adds support for case-insensitive filtering; so lowercase names are definitely possible.

godexsoft

Just leaving a suggestion. Looks good overall 👍

godexsoft · 2025-02-07T16:55:09Z

src/xrpld/rpc/detail/RPCHelpers.cpp

-        static constexpr auto types =
-            std::to_array<std::pair<char const*, LedgerEntryType>>({
+        static constexpr auto types = std::to_array<
+            std::tuple<char const*, char const*, LedgerEntryType>>({


nit: For readability, consider a small struct instead of the tuple.

I figured that if we're going to deprecate the current way of referencing ledger entries, then the next release we can return to using a std::pair - which would be preferred over a dedicated struct.

Although the RPC name-based deprecation is no longer going to happen, I looked at this again and think that adding a new struct just for this function is overkill, however small that struct may be.

bthomee · 2025-02-07T18:50:00Z

Maybe this is an unpopular opinion but I actually like the lower-case names, and think we should keep them.
note: I have no objection to adding additional filter names, just to deprecating/replacing the existing setup.

I have no particular opinion here or sufficient background to know how people use this API, but do note that this PR explicitly adds support for case-insensitive filtering; so lowercase names are definitely possible.

@mDuo13 & @mvadari, let's discuss whether or not to deprecate the existing setup. In particular, @mDuo13 raised some good points, so @mvadari do you feel strongly about that we should keep the existing, frequently odd, naming convention & the increased burden in remembering to add such a separate name for each new ledger entry type?

mvadari · 2025-02-07T18:53:30Z

Maybe this is an unpopular opinion but I actually like the lower-case names, and think we should keep them.
note: I have no objection to adding additional filter names, just to deprecating/replacing the existing setup.

I have no particular opinion here or sufficient background to know how people use this API, but do note that this PR explicitly adds support for case-insensitive filtering; so lowercase names are definitely possible.

@mDuo13 & @mvadari, let's discuss whether or not to deprecate the existing setup. In particular, @mDuo13 raised some good points, so @mvadari do you feel strongly about that we should keep the existing, frequently odd, naming convention & the increased burden in remembering to add such a separate name for each new ledger entry type?

The rpcName got added to the ledger entry macro in #5202, so that should make it much easier to keep everything aligned.

ximinez · 2025-02-10T22:56:34Z

IMHO, the way forward here is to preserve the backward compatibility on the existing "short names" indefinitely, but deprecate adding new ones, or at least making it optional.

Instead of marking anything deprecated, the documentation could, for example, include a list of values that can be used to query each type, and as we add new types, only include the canonical name.

intelliot

When using existing public Devnet servers, I would get:

RippledError: Invalid field 'type'.
{
  data: {
    api_version: 2,
    error: 'invalidParams',
    error_code: 31,
    error_message: "Invalid field 'type'.",
    id: 2,
    request: {
      api_version: 2,
      command: 'ledger_data',
      id: 2,
      type: 'MPTokenIssuance'
    },
    status: 'error',
    type: 'response'
  }
}

I compiled and ran this PR on Devnet. The same query works great! 👍

Thank you for this!

bthomee · 2025-02-11T21:21:04Z

When using existing public Devnet servers, I would get:

RippledError: Invalid field 'type'.
{
  data: {
    api_version: 2,
    error: 'invalidParams',
    error_code: 31,
    error_message: "Invalid field 'type'.",
    id: 2,
    request: {
      api_version: 2,
      command: 'ledger_data',
      id: 2,
      type: 'MPTokenIssuance'
    },
    status: 'error',
    type: 'response'
  }
}

I compiled and ran this PR on Devnet. The same query works great! 👍

Thank you for this!

Great to hear! I'll follow @ximinez's and @mvadari's suggestions to keep this backward compatible, and will make changes accordingly soon.

bthomee · 2025-02-12T14:58:08Z

IMHO, the way forward here is to preserve the backward compatibility on the existing "short names" indefinitely, but deprecate adding new ones, or at least making it optional.

Instead of marking anything deprecated, the documentation could, for example, include a list of values that can be used to query each type, and as we add new types, only include the canonical name.

I've removed the deprecation notices. In terms of the documentation, while it can be added to the code here, I think it would be better to have the possible ledger entry values documented in the various libraries that contact rippled over RPC.

bthomee added 2 commits February 1, 2025 12:47

Support canonical ledger entry names

312a5a4

Update API-CHANGELOG and add deprecation note

9b501bd

bthomee mentioned this pull request Feb 1, 2025

Support canonical ledger entry names in ledger/ledger_data/account_objects "type" field. #4393

Open

bthomee requested review from mvadari, ximinez, intelliot and mDuo13 February 1, 2025 21:38

bthomee requested a review from godexsoft February 4, 2025 18:18

godexsoft approved these changes Feb 7, 2025

View reviewed changes

intelliot approved these changes Feb 11, 2025

View reviewed changes

bthomee and others added 2 commits February 12, 2025 08:57

Merge branch 'develop' into bthomee/ledgertypefilter

2b11eb0

Remove deprecation notice

6b18280

bthomee added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 12, 2025

Merge branch 'develop' into bthomee/ledgertypefilter

7a35881

bthomee merged commit 7c9d652 into develop Feb 14, 2025
24 checks passed

bthomee deleted the bthomee/ledgertypefilter branch February 14, 2025 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support canonical ledger entry names #5271

Support canonical ledger entry names #5271

bthomee commented Feb 1, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

mvadari commented Feb 3, 2025

bthomee commented Feb 3, 2025

mDuo13 commented Feb 3, 2025 •

edited

Loading

mvadari commented Feb 5, 2025 •

edited

Loading

bthomee commented Feb 5, 2025

godexsoft left a comment

godexsoft Feb 7, 2025

bthomee Feb 7, 2025

bthomee Feb 12, 2025

bthomee commented Feb 7, 2025

mvadari commented Feb 7, 2025

ximinez commented Feb 10, 2025

intelliot left a comment

bthomee commented Feb 11, 2025

bthomee commented Feb 12, 2025

Support canonical ledger entry names #5271

Support canonical ledger entry names #5271

Conversation

bthomee commented Feb 1, 2025

High Level Overview of Change

Context of Change

Type of Change

API Impact

Before / After

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

mvadari commented Feb 3, 2025

bthomee commented Feb 3, 2025

mDuo13 commented Feb 3, 2025 • edited Loading

mvadari commented Feb 5, 2025 • edited Loading

bthomee commented Feb 5, 2025

godexsoft left a comment

Choose a reason for hiding this comment

godexsoft Feb 7, 2025

Choose a reason for hiding this comment

bthomee Feb 7, 2025

Choose a reason for hiding this comment

bthomee Feb 12, 2025

Choose a reason for hiding this comment

bthomee commented Feb 7, 2025

mvadari commented Feb 7, 2025

ximinez commented Feb 10, 2025

intelliot left a comment

Choose a reason for hiding this comment

bthomee commented Feb 11, 2025

bthomee commented Feb 12, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

mDuo13 commented Feb 3, 2025 •

edited

Loading

mvadari commented Feb 5, 2025 •

edited

Loading